Add variable composition and Handlebars template rendering#1731
Add variable composition and Handlebars template rendering#1731
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Deploying logfire-docs with
|
| Latest commit: |
9d5a27e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://041a8614.logfire-docs.pages.dev |
| Branch Preview URL: | https://feature-variable-composition.logfire-docs.pages.dev |
| def _render_json_value(value: Any, hbs_render: Callable[..., str], context: dict[str, Any]) -> Any: | ||
| """Recursively render Handlebars templates in a decoded JSON value. | ||
|
|
||
| Only string values are rendered; dicts and lists are walked recursively. | ||
| """ | ||
| if isinstance(value, str): | ||
| return hbs_render(value, context) | ||
| if isinstance(value, dict): | ||
| return {k: _render_json_value(v, hbs_render, context) for k, v in value.items()} # pyright: ignore[reportUnknownVariableType] | ||
| if isinstance(value, list): | ||
| return [_render_json_value(item, hbs_render, context) for item in value] # pyright: ignore[reportUnknownVariableType] | ||
| # Numbers, booleans, None pass through unchanged | ||
| return value |
There was a problem hiding this comment.
🚩 render_serialized_string renders ALL string values, not just those with templates
The _render_json_value function at logfire/variables/abstract.py:279-291 renders every string in the decoded JSON through the Handlebars engine, even strings that don't contain {{}} syntax. For values without templates, this is a no-op (the Handlebars engine returns the input string unchanged), so it's functionally correct but adds unnecessary overhead. More importantly, if a non-template string field happens to contain {{ and }} (e.g., a regex pattern, code snippet, or API key), it would be silently modified. This is by design for TemplateVariable where all string fields are templates, but users of ResolvedVariable.render() on regular Variable instances should be aware of this behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _render_default(self, default: Any, render_fn: Callable[[str], str]) -> T_co: | ||
| """Serialize the default value, apply render_fn, then deserialize back.""" | ||
| try: | ||
| serialized = self.type_adapter.dump_json(default).decode('utf-8') | ||
| rendered = render_fn(serialized) | ||
| result = self._deserialize(rendered) | ||
| if isinstance(result, Exception): | ||
| raise result | ||
| return result | ||
| except Exception: | ||
| # If rendering the default fails, return the original default | ||
| return default |
There was a problem hiding this comment.
🚩 _render_default silently swallows rendering errors and returns unrendered default
In logfire/variables/variable.py:273-284, the _render_default method catches all exceptions with a bare except Exception and returns the original unrendered default value. This means if the default value contains invalid Handlebars syntax (e.g., 'Hello {{#if}}' missing closing tag) or if serialization fails, the user receives the raw template string with no indication that rendering failed. The ResolvedVariable returned in this path (via the context override or default fallback) has no exception field set, so the failure is completely invisible. This design choice avoids crashes but could lead to subtle bugs where users see {{placeholder}} in output without understanding why.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _unescape_protected(s: str) -> str: | ||
| """Undo only the four entities we introduced.""" | ||
| return s.replace('{', '{').replace('}', '}').replace('<', '<').replace('>', '>') |
There was a problem hiding this comment.
🚩 Swap-based <<>> rendering assumes no natural &#NNN; entities in templates
The angle_bracket.py swap algorithm protects context values by replacing {, }, <, > with HTML numeric entities ({ etc.), then swaps {↔< and }↔> in the template, renders with standard Handlebars, reverse-swaps, and unescapes the entities. This works correctly as long as the template and context values don't already contain the exact entity sequences {, }, <, or >. If a context value naturally contains e.g. <, the _unescape_protected step at logfire/handlebars/angle_bracket.py:44 would incorrectly convert it to <. This is unlikely in practice (variable values are typically plain text or JSON, not pre-escaped HTML), but worth noting for edge cases involving HTML content in variables.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Recursively expand references within the resolved value (if it's a string). | ||
| nested_composed: list[ComposedReference] = [] | ||
| if isinstance(raw_value, str) and has_references(json.dumps(raw_value)): | ||
| try: | ||
| expanded_serialized, nested_composed = expand_references( | ||
| json.dumps(raw_value), | ||
| ref_name, | ||
| resolve_fn, | ||
| _visited=visited, | ||
| _depth=_depth + 1, | ||
| ) | ||
| raw_value = json.loads(expanded_serialized) | ||
| except VariableCompositionError as e: | ||
| composed.append( | ||
| ComposedReference( | ||
| name=ref_name, | ||
| value=None, | ||
| label=ref_label, | ||
| version=ref_version, | ||
| reason=ref_reason, | ||
| error=str(e), | ||
| ) | ||
| ) | ||
| unresolved_names.add(ref_name) | ||
| continue |
There was a problem hiding this comment.
🚩 expand_references only recursively expands string-valued references, not structured ones
At logfire/variables/composition.py:185, the recursive expansion check isinstance(raw_value, str) and has_references(...) means that if a referenced variable resolves to a dict/list containing strings with <<refs>>, those nested string references are not recursively expanded. Only top-level string values get recursive expansion. The _render_value function at line 312 does walk dicts/lists and render each string through Handlebars, but this only handles the current variable's value — it doesn't recursively resolve references within composed dict values. This is likely intentional (composition is for string interpolation, not deep object merging), but could surprise users who reference a structured variable expecting its internal <<refs>> to also be expanded.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _protect_value(value: Any) -> Any: | ||
| """Recursively protect string values, preserving structure for dicts/lists.""" | ||
| if isinstance(value, str): | ||
| return SafeString(value.translate(_PROTECT)) | ||
| if isinstance(value, dict): | ||
| return {k: _protect_value(v) for k, v in value.items()} # pyright: ignore[reportUnknownVariableType] | ||
| if isinstance(value, list): | ||
| return [_protect_value(v) for v in value] # pyright: ignore[reportUnknownVariableType] | ||
| return value # bools, ints, None, etc. — pass through |
There was a problem hiding this comment.
🟡 _unescape_protected corrupts context values that contain literal HTML entity strings
In logfire/handlebars/angle_bracket.py:44, the _unescape_protected function unconditionally replaces {, }, <, and > in the entire rendered output. This corrupts context values that legitimately contain these literal entity strings.
Root Cause
The _protect_value function at logfire/handlebars/angle_bracket.py:47-55 translates {}<> characters in context values to HTML entities (e.g., { → {). After Handlebars rendering, _unescape_protected reverses this. However, _protect_value does not protect pre-existing entity strings in context values.
For example, if a context value is the string "Use { for curly braces", _protect_value sees no {}<> characters and returns it unchanged. But _unescape_protected then converts { to {, producing "Use { for curly braces" — corrupting the original value.
Confirmed by test:
render_once('Value: <<val>>', {'val': 'Use { for curly braces'})
# Returns: 'Value: Use { for curly braces'
# Expected: 'Value: Use { for curly braces'Impact: Variable values containing literal HTML numeric entities {, }, <, or > will be silently corrupted during <<>> composition. This is uncommon in typical variable values but could occur in HTML-related configuration strings.
Prompt for agents
In logfire/handlebars/angle_bracket.py, the _protect_value function (lines 47-55) needs to also escape pre-existing HTML entity strings that match the four entities used by the protection scheme ({, }, <, >) BEFORE translating {}<> to those entities. This way, _unescape_protected will only reverse the entities that _protect_value introduced, not pre-existing ones.
One approach: in _protect_value, first replace any existing {/}/</> with a unique escape sequence (e.g., double them or use a different encoding), then apply the {}<> translation. In _unescape_protected, reverse only the single-encoded entities, then restore the pre-existing ones.
Alternatively, use a different set of placeholder strings that are extremely unlikely to appear in user content (e.g., using Unicode private use area characters or a unique sentinel prefix).
Was this helpful? React with 👍 or 👎 to provide feedback.
| except ( # Safety net: providers and resolve functions are user-defined and may raise any of these | ||
| ValidationError, | ||
| ValueError, | ||
| TypeError, | ||
| KeyError, | ||
| AttributeError, | ||
| RuntimeError, | ||
| OSError, | ||
| HandlebarsError, | ||
| VariableCompositionError, | ||
| ) as e: | ||
| if span and serialized_result is not None: # pragma: no cover | ||
| span.set_attribute('invalid_serialized_label', serialized_result.label) | ||
| span.set_attribute('invalid_serialized_value', serialized_result.value) | ||
| default = self._get_default(targeting_key, attributes) | ||
| return ResolvedVariable(name=self.name, value=default, exception=e, _reason='other_error') |
There was a problem hiding this comment.
🚩 Broad except clause narrowed but may still miss some exception types
The safety net in _resolve at logfire/variables/variable.py:267-277 catches a specific list of exception types. This was narrowed from a bare except Exception in the previous code. The list includes ValidationError, ValueError, TypeError, KeyError, AttributeError, RuntimeError, OSError, HandlebarsError, and VariableCompositionError. This is a reasonable set for user-defined providers and resolve functions, but if a provider raises something like ConnectionError (which doesn't inherit from OSError on all paths) or TimeoutError, those would propagate uncaught. The comment says these are user-defined providers, so this is a deliberate design choice to let truly unexpected errors surface rather than silently falling back to defaults.
Was this helpful? React with 👍 or 👎 to provide feedback.
| from logfire.variables.composition import ( | ||
| ComposedReference, | ||
| VariableCompositionCycleError, | ||
| VariableCompositionError, | ||
| ) |
There was a problem hiding this comment.
🔴 Eager import of pydantic_handlebars in __init__.py breaks import logfire when the optional dependency is not installed
The new top-level import in logfire/variables/__init__.py creates an eager import chain that causes import logfire to fail with ImportError for any user who doesn't have pydantic-handlebars installed (which is most users, since it's only in the [variables] optional extra, and only for Python 3.10+).
Root Cause and Import Chain
The import chain is:
import logfire→from . import variables as variables(logfire/__init__.py:10)logfire/variables/__init__.py:16-20eagerly importsfrom logfire.variables.composition import ComposedReference, ...logfire/variables/composition.py:22→from logfire.variables.angle_bracket import render_oncelogfire/variables/angle_bracket.py:21→from pydantic_handlebars import SafeString, render as hbs_renderImportError: No module named 'pydantic_handlebars'
Before this PR, composition.py and angle_bracket.py did not exist, and __init__.py only imported from composition lazily or not at all. The new lines 16-20 in __init__.py are NOT inside the existing __getattr__ lazy-import block — they execute immediately when logfire.variables is imported.
pydantic-handlebars is declared as an optional dependency in pyproject.toml:87:
variables = ["pydantic>=2", "pydantic-handlebars>=0.1.0; python_version >= '3.10'"]
So it is not installed for:
- Users who
pip install logfirewithout the[variables]extra - Users on Python 3.9 who
pip install logfire[variables](the marker excludes 3.9)
Impact: import logfire crashes for the majority of users who don't install the [variables] extra, completely breaking the SDK.
Prompt for agents
Move the eager imports of ComposedReference, VariableCompositionCycleError, and VariableCompositionError from the top level of logfire/variables/__init__.py (lines 16-20) into the lazy __getattr__ function (around line 114-127). These classes should be imported alongside Variable, TemplateVariable, etc. inside the __getattr__ block to avoid triggering the pydantic_handlebars import chain when the package is loaded.
Specifically:
1. In logfire/variables/__init__.py, remove lines 16-20 (the top-level 'from logfire.variables.composition import ...')
2. Add the composition imports inside the __getattr__ function body (around line 122), alongside the other lazy imports from logfire.variables.variable and logfire.variables.config.
3. Also add them to the TYPE_CHECKING block (around line 40-49) so type checkers still see them.
Additionally, in logfire/variables/angle_bracket.py (line 21) and logfire/variables/variable.py (line 14), the pydantic_handlebars imports should be made lazy (e.g., import inside functions or behind try/except ImportError) so that basic variable functionality works without pydantic-handlebars installed, matching the documented Python 3.9 support for logfire.var() without templates.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| from opentelemetry.trace import get_current_span | ||
| from pydantic import TypeAdapter, ValidationError | ||
| from pydantic_handlebars import HandlebarsError |
There was a problem hiding this comment.
🚩 variable.py also unconditionally imports pydantic_handlebars at module level
In addition to the import chain through __init__.py → composition.py → angle_bracket.py (reported as BUG-0001), logfire/variables/variable.py:14 has from pydantic_handlebars import HandlebarsError at the top level. This module is lazily imported via __getattr__ in __init__.py, so it only triggers when someone actually accesses Variable or TemplateVariable. This is less severe than the eager chain in __init__.py, but it still means basic logfire.var() (which doesn't need Handlebars) would fail on Python 3.9 or without the [variables] extra. The HandlebarsError import should be made lazy (e.g., imported inside the exception handler or behind a conditional).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| from opentelemetry.trace import get_current_span | ||
| from pydantic import TypeAdapter, ValidationError | ||
| from pydantic_handlebars import HandlebarsError |
There was a problem hiding this comment.
🔴 Unconditional top-level from pydantic_handlebars import HandlebarsError in variable.py breaks variable usage on Python 3.9
The new top-level import from pydantic_handlebars import HandlebarsError in logfire/variables/variable.py is unconditional, causing an ImportError when the module is loaded on Python 3.9 where pydantic-handlebars is never installed.
Root Cause and Impact
At logfire/variables/variable.py:14, the new line:
from pydantic_handlebars import HandlebarsErroris executed at module load time. The pydantic-handlebars package is gated with python_version >= '3.10' in pyproject.toml:87, so it's never available on Python 3.9.
While variable.py is lazily loaded via __getattr__ in logfire/variables/__init__.py, it IS loaded whenever a user accesses Variable, TemplateVariable, targeting_context, or ResolveFunction. This means that even basic non-template variable usage (logfire.var(name='x', default=True)) will fail on Python 3.9.
Impact: All variable features are completely broken on Python 3.9, contradicting the documented guarantee that "basic variable features (logfire.var() without templates or composition) still work."
The fix should guard this import — either move it to be lazy (imported where needed) or use a try/except to define a fallback, so that HandlebarsError is only required when template features are actually used.
| from pydantic_handlebars import HandlebarsError | |
| try: | |
| from pydantic_handlebars import HandlebarsError | |
| except ImportError: # pydantic-handlebars requires Python 3.10+ | |
| HandlebarsError = None # type: ignore[assignment,misc] |
Was this helpful? React with 👍 or 👎 to provide feedback.
…ame>> reference expansion, and template validation
dmontagu
force-pushed
the
feature/variable-composition
branch
from
d414e21 to
9d5a27e
Compare
| from dataclasses import dataclass, field | ||
| from typing import Any, Callable, Optional, Tuple # noqa: UP035 | ||
|
|
||
| from logfire.variables.angle_bracket import render_once |
There was a problem hiding this comment.
🔴 Unconditional top-level import of pydantic_handlebars in composition.py via angle_bracket
composition.py line 22 has from logfire.variables.angle_bracket import render_once at the top level. angle_bracket.py line 21 does from pydantic_handlebars import SafeString, render as hbs_render. This means importing composition.py — which is needed for basic variable operations like logfire.var() — will fail on Python 3.9 or without pydantic-handlebars installed. The render_once function is only used inside _render_value() (line 322), so it could be imported lazily there.
| from logfire.variables.angle_bracket import render_once |
Was this helpful? React with 👍 or 👎 to provide feedback.
| def var(self, name: str, *, default: T, description: str | None = None, template_inputs: type[Any] | None = None) -> Variable[T]: ... | ||
| @overload | ||
| def var(self, name: str, *, type: type[T], default: T | ResolveFunction[T], description: str | None = None) -> Variable[T]: ... | ||
| def var(self, name: str, *, type: type[T], default: T | ResolveFunction[T], description: str | None = None, template_inputs: type[Any] | None = None) -> Variable[T]: ... |
There was a problem hiding this comment.
🚩 logfire-api stubs don't include template_var method
The logfire-api package stubs in logfire-api/logfire_api/_internal/main.pyi do not include a template_var method definition, though var() was updated with the new template_inputs parameter. Per CLAUDE.md, the .pyi stubs are autogenerated during release and should be ignored. This is not a bug but the no-op shim package (logfire-api/logfire_api/__init__.py) may also need updating if template_var should be available there — worth checking if test_logfire_api.py passes.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
<<variable_name>>references in serialized variable values via literal string substitution before deserialization. Supports nested references with cycle/depth detection, escaping, and tracking of composed references in span attributes.logfire/handlebars/) supporting{{placeholder}}syntax with helpers (#if,#each,#unless,#with,lookup,log, etc.), whitespace control, subexpressions, and safe string escaping.TemplateVariable[T, InputsT]: New variable type whereget(inputs)resolves the value, expands<<ref>>composition, renders{{placeholder}}templates via Handlebars, and deserializes — all in a single call. Factory methodlogfire.template_var()added.logfire/variables/template_validation.py):find_template_fields,validate_template_composition, anddetect_composition_cyclesfor backend validation of template references.New modules
logfire/variables/composition.pyexpand_references(),find_references(),ComposedReferencelogfire/variables/template_validation.pylogfire/handlebars/Test plan
tests/test_variable_composition.py— 43 tests covering simple/nested/cyclic refs, escaping, JSON edge cases, structured types, span attributestests/test_variable_templates.py— tests for TemplateVariable, Handlebars rendering, override behavior, structured model templatestests/test_template_validation.py— 58 tests for template field validation, cycle detection, edge casestests/test_handlebars/— 157 tests for render, compile, helpers, tokenizer, whitespace, security, subexpressions, environment